Introducing UsageDetails and Aggregated Cost by Managment Group Scope. Also updated some existing examples to include missing billingperiod in billingPeriodId Property. by parisa-naeimi · Pull Request #3588 · Azure/azure-rest-api-specs

parisa-naeimi · 2018-08-06T21:15:44Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

… Account) Marketplace, Usage Details, and Enrollment Level Balances.

…laced priceHidden boolean type with enum for more clarity

…er-location to some parameters missing that part.

# Conflicts: # specification/consumption/resource-manager/Microsoft.Consumption/stable/2018-03-31/consumption.json # specification/consumption/resource-manager/Microsoft.Consumption/stable/2018-03-31/examples/CostTags.json # specification/consumption/resource-manager/Microsoft.Consumption/stable/2018-03-31/examples/CreateOrUpdateCostTags.json

…. Also updated some existing examples to include missing "billingperiod" in "billingPeriodId" Property.

AutorestCI · 2018-08-06T21:16:25Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-08-06T21:16:26Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#3319

AutorestCI · 2018-08-06T21:16:26Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2158

AutorestCI · 2018-08-06T21:16:58Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

azuresdkci · 2018-08-06T21:25:07Z

Can one of the admins verify this patch?

parisa-naeimi · 2018-08-07T18:45:16Z

@marstr Hello Martin, Please take a look at this PR and let me know if I need to do something here. Also this PR needs ARMReview. Thank you

marstr

I have a few comments that should keep your operation groups consistent through-out this swagger.

In general the pattern here does feel kind of backwards here, shouldn't there be an operation group per scope that can have consumption data viewed? Then have UsageDetails and AggregatedCost operations from there? It's not actionable ATM, because it would cause breaking changes without just cause. However, it may be food for thought in future API Versions.

marstr · 2018-08-07T19:17:46Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-06-30/consumption.json

+        "tags": [
+          "UsageDetails"
+        ],
+        "operationId": "UsageDetailsByManagementGroup_List",


Please update this to UsageDetails_ListByManagementGroup to reduce the number of separate clients generated by AutoRest.

Sure, will apply your comment

marstr · 2018-08-07T19:20:28Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-06-30/consumption.json

+        "tags": [
+          "AggregatedCost"
+        ],
+        "operationId": "AggregatedcostByManagementGroup",


Please add an Operation Group here. I'd have this operationID be: AggregatedCost_ByManagementGroup

Sure, will apply it

marstr · 2018-08-07T20:39:00Z

...cation/consumption/resource-manager/Microsoft.Consumption/stable/2018-06-30/consumption.json

+        "tags": [
+          "UsageDetails"
+        ],
+        "operationId": "UsageDetails_ListByForBillingPeriodByManagementGroup",


ListByForBillingPeriodByManagementGroup seems like a typo to me. Was it supposed to be ListByBillingPeriodByManagementGroup?

Yeah, It is a typo Good Catch 👍

marstr

Please fix the individual comments noted above.

parisa-naeimi · 2018-08-07T21:47:47Z

@marstr I addressed all of your comments. Thanks again.

marstr

Look like the linter has a few good suggestions that should be fixed up. Both the missing x-ms-parameter-location on the global parameter and making sure that Get is added to those OperationIDs we were looking at before. Sorry, I should have caught the missing Get or List earlier!

parisa-naeimi · 2018-08-07T22:54:09Z

@marstr No worries, I will fix those ones as well. Is it the reason it is failing? cause these ones seem to me as a warning

marstr · 2018-08-07T22:57:19Z

Is it the reason it is failing?

Adding new warnings or errors causes the job to fail, but we only block on it if the new warnings or errors are needless. This time we should fix them up, even though they're just warnings :)

parisa-naeimi · 2018-08-07T22:59:03Z

Can you give me an example for using location parameter? what do you expect to see there?

…ethod name of AggregatedCost

marstr · 2018-08-07T23:06:35Z

Yeah, sorry it looks like the documentation the linter links to is missing this rule for some reason. :/ I'll look into that.

Anyway, here's an example of the SQL team using it:

azure-rest-api-specs/specification/sql/resource-manager/Microsoft.Sql/preview/2017-10-01-preview/cancelPoolOperations.json

Line 259 in 0b33b0b

"x-ms-parameter-location": "method"

It needs to be applied on any document-scoped parameter definition so that AutoRest knows whether or not to ask for them as part of a client constructor or in the methods that are generated. You can read more about it here: https://github.com/Azure/autorest/tree/master/docs/extensions#x-ms-parameter-location

parisa-naeimi · 2018-08-07T23:11:24Z

@marstr Thank you, I fixed those and sent another iteration

AutorestCI · 2018-08-07T23:16:51Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#2433

marstr

LGTM, now we'll just need to wait for the ARM team.

parisa-naeimi · 2018-08-07T23:21:41Z

@marstr Thank you very much 👍

ravbhatnagar · 2018-08-09T06:47:35Z

Looks good

marstr · 2018-08-09T19:22:10Z

Ready for me to merge, @parisa-naeimi?

parisa-naeimi · 2018-08-09T19:23:59Z

Sure, sounds good to me. Thank you @marstr

parisa-naeimi added 11 commits April 11, 2018 22:15

This PR includes above-subscription level (Enrollment, Department, or…

0d10889

… Account) Marketplace, Usage Details, and Enrollment Level Balances.

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

e8a464b

Updated consumption.json to fix build failure issues

247831c

Added "Get" to Operation Name 'BalancesByBillingAccount' and also rep…

a198973

…laced priceHidden boolean type with enum for more clarity

Added Enrollment-Level balances by BillingPeriod

c70d23f

Updated BalancesByBillingAccount example files and added x-ms-paramet…

f00a550

…er-location to some parameters missing that part.

Replaced Microsoft.CostManagment with Microsoft.Billing

876f31a

Fixed example errors

d53bf42

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

f6fda21

Introducing UsageDetails and Aggregated Cost by Managment Group Scope…

1275106

…. Also updated some existing examples to include missing "billingperiod" in "billingPeriodId" Property.

AutorestCI mentioned this pull request Aug 6, 2018

[AutoPR consumption/resource-manager] Introducing UsageDetails and Aggregated Cost by Managment Group Scope. Also updated some existing examples to include missing billingperiod in billingPeriodId Property. Azure/azure-sdk-for-python#3085

Merged

azuresdkci assigned marstr Aug 6, 2018

azuresdkci requested a review from marstr August 6, 2018 21:16

marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 7, 2018

marstr reviewed Aug 7, 2018

View reviewed changes

marstr requested changes Aug 7, 2018

View reviewed changes

Addressed comments from review

f7e522f

AutorestCI mentioned this pull request Aug 7, 2018

[AutoPR consumption/resource-manager] Introducing UsageDetails and Aggregated Cost by Managment Group Scope. Also updated some existing examples to include missing billingperiod in billingPeriodId Property. Azure/azure-sdk-for-node#3305

Merged

marstr requested changes Aug 7, 2018

View reviewed changes

Added location parameter to managementgroupId and also added Get To m…

0dc16e7

…ethod name of AggregatedCost

marstr mentioned this pull request Aug 7, 2018

Missing Documentation for Validation Rule R4001 #3604

Closed

AutorestCI mentioned this pull request Aug 7, 2018

[AutoPR consumption/resource-manager] Introducing UsageDetails and Aggregated Cost by Managment Group Scope. Also updated some existing examples to include missing billingperiod in billingPeriodId Property. Azure/azure-sdk-for-go#2418

Merged

marstr approved these changes Aug 7, 2018

View reviewed changes

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 9, 2018

marstr merged commit f030ddc into Azure:master Aug 9, 2018

Conversation

parisa-naeimi commented Aug 6, 2018

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

AutorestCI commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-java

Uh oh!

azuresdkci commented Aug 6, 2018

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

marstr left a comment

Choose a reason for hiding this comment

Uh oh!

marstr Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

marstr Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

marstr Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

marstr left a comment

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

marstr left a comment

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

marstr commented Aug 7, 2018

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

marstr commented Aug 7, 2018

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

AutorestCI commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

marstr left a comment

Choose a reason for hiding this comment

Uh oh!

parisa-naeimi commented Aug 7, 2018

Uh oh!

ravbhatnagar commented Aug 9, 2018

Uh oh!

marstr commented Aug 9, 2018

Uh oh!

parisa-naeimi commented Aug 9, 2018

Uh oh!

Reviewers

AutorestCI commented Aug 6, 2018 •

edited

Loading

AutorestCI commented Aug 6, 2018 •

edited

Loading

AutorestCI commented Aug 6, 2018 •

edited

Loading

AutorestCI commented Aug 6, 2018 •

edited

Loading

AutorestCI commented Aug 7, 2018 •

edited

Loading